[SYCL][Ext][Bindless] Initial implementation of image spirv builtins on HIP #16439
Conversation
GeorgeWeb
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
…on HIP TODO: Complete the description with more information. Signed-off-by: Georgi Mirazchiyski <[email protected]>
GeorgeWeb
force-pushed
the
georgi/bindless-hip
branch
from
9384f74 to
437f972
Compare
GeorgeWeb
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
GeorgeWeb
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
I appreciate that |
| #endif | ||
|
|
||
| #ifdef _WIN32 | ||
| #define _CLC_MANGLE_FUNC_IMG_HANDLE(namelength, name, prefix, postfix) \ |
There was a problem hiding this comment.
Isn't that the kind of thing that the remangler should handle, as in m - y, long (long) unsigned?
There was a problem hiding this comment.
@jchlanda I agree that it should probably be the case.
We have that tendency of manually mangling these, for example, also in the atomics libspirv function implementations. This particular case here is adapted from the libspirv ptx image implementation. I think it is the perfect candidate to create a follow-up for, where we attempt to cleanup some of the manual mangling (at least for the image functions) across both ptx and amdgcn libspirv target implementations. I'll create an internal ticket for our team to follow this up. Does that sound okay?
I think that would be a good idea. |
|
|
||
| // RUN: %{build} -o %t.out | ||
| // RUN: %{run} %t.out | ||
| // RUN: %if !any-device-is-hip %{ %{build} -o %t.out %} |
There was a problem hiding this comment.
Is this not the same as doing // UNSUPPORTED: HIP? If so, would that style not be better?
There was a problem hiding this comment.
Yeah it absolutely is. That's a weird leftover I didn't spot. Should be changed to // UNSUPPORTED: HIP with a reasonable UNSUPPORTED-INTEDED description, and will do. (Think I did that before knowing I had to do UNSUPPORTED-INTEDED for unsupported targets 😅)
Signed-off-by: Georgi Mirazchiyski <[email protected]>
GeorgeWeb
force-pushed
the
georgi/bindless-hip
branch
from
eadd947 to
e5e0d35
Compare
| // REQUIRES: aspect-ext_oneapi_bindless_images | ||
|
|
||
| // UNSUPPORTED: hip || level_zero | ||
| // UNSUPPORTED-INTENDED: Returning non-FP values from fetching fails on HIP. |
There was a problem hiding this comment.
| // UNSUPPORTED-INTENDED: Returning non-FP values from fetching fails on HIP. | |
| // UNSUPPORTED-INTENDED: Returning non-FP values from sampling fails on HIP. |
Fetching implies no sampling is occuring.
This PR implements the required spirv builtins in the libclc for AMD targets to support image fetching (excluding sampled fetch for now) and sampling as well as image array fetching and sampling.
Additionally, End-to-end tests are updated to require the aspects corresponding to the feature that is being tested from the Bindless Images extension. This helps avoid having to manually say which backend adapter is supported or unsupported and instead rely on support based on aspect/device queries to drive the execution of the tests.
HIP adapter implementation in UR: oneapi-src/unified-runtime#2496
Signed-off-by: Georgi Mirazchiyski [email protected]